[exporterhelper] Make the re-enqueue behavior configurable #8993
Conversation
dmitryax
force-pushed
the
requeue-config-option
branch
from
5fe217b
to
7ab6933
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8993 +/- ##
==========================================
- Coverage 91.57% 91.56% -0.01%
==========================================
Files 316 316
Lines 17147 17138 -9
==========================================
- Hits 15702 15693 -9
Misses 1150 1150
Partials 295 295 ☔ View full report in Codecov by Sentry. |
dmitryax
force-pushed
the
requeue-config-option
branch
from
7ab6933
to
a7685b0
Compare
|
Needs a rebase. Also I am not convinced that this is clear to the users, I am worried that will get them more confused. |
dmitryax
force-pushed
the
requeue-config-option
branch
from
a7685b0
to
56096c6
Compare
|
@bogdandrutu I rebased and updated the config option description. PTAL if it's more clear now |
dmitryax
force-pushed
the
requeue-config-option
branch
from
56096c6
to
4658926
Compare
| // If true, a request failed with a non-permanent error will be put back in the queue and | ||
| // retried after the current queue is drained. | ||
| // If false (default), all failed requests will be dropped. | ||
| RequeueOnFailure bool `mapstructure:"requeue_on_failure"` |
There was a problem hiding this comment.
requeue_on_transient_failure seems like a lot of typing. Also we have retry_on_failure, so we probably should stay consistent
dmitryax
force-pushed
the
requeue-config-option
branch
from
4658926
to
526cdf6
Compare
dmitryax
force-pushed
the
requeue-config-option
branch
from
526cdf6
to
de80498
Compare
Instead of relying or enabled `retry_on_failure` option, we now have a new option `sending_queue::requeue_on_failure` to control the requeuing independently of the retry sender. This can be useful for users who don't want the blocking exponential retry, just want to put the failed request back to the queue. This option can also be enabled with memory queue now, which means that the data will never be dropped after getting to the queue as long as the collector is up and running.
dmitryax
force-pushed
the
requeue-config-option
branch
from
de80498
to
4b815ff
Compare
| // If true, a request failed with a non-permanent error will be put back in the queue and | ||
| // retried after the current queue is drained. | ||
| // If false (default), all failed requests will be dropped. | ||
| RequeueOnFailure bool `mapstructure:"requeue_on_failure"` |
There was a problem hiding this comment.
Any reasons we want requeue_on_failure rather than drop_data_on_failure?
- By default, users don't like data loss. If they miss out on this configuration they would have data loss unexpectedly.
- Having
drop_data_on_failurewith default to false would make this change backward compatible.
There was a problem hiding this comment.
But that's the case for the persistent queue only. Memory queue has the opposite behavior.
There was a problem hiding this comment.
It's not even always applicable to the persistent queue. Only if retry_on_failure::enabled=true
There was a problem hiding this comment.
We can probably do some transition period when this option is preset if persistent queue + retry_on_failure are enabled with a warning asking users to set this explicitly
There was a problem hiding this comment.
hmm i see. so this is also aligning the behavior between memory/persistent queue. yeah having some WARN logs would probably be helpful in this transition time.
|
#9054 should be merged first, otherwise, requeue to the memory queue on shutdown potentially can panic |
|
We decided to remove requeuing instead #9090 |
Introduce a new option
sending_queue::requeue_on_failureto control the requeuing independently of the retry sender. This can be useful for users who don't want the blocking exponential retry and just want to put the failed request back in the queue. This option can also be enabled with the memory queue now, which means that the data will never be dropped after getting to the queue as long as the collector is up and running.Resolves #8987